Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use auth0 as our login mechanism #245

Merged
13 commits merged into from
Feb 17, 2023
Merged

use auth0 as our login mechanism #245

13 commits merged into from
Feb 17, 2023

Conversation

ghost
Copy link

@ghost ghost commented Feb 16, 2023

Big Honkin' Feature Merge™. Sorry. :-(

This rips out all of the current analytics tracking, and replaces it with an auth0-based approach. There's a description of the overall flow at klothoplatform/klotho-auth-service. If you like a more hands-on approach, you can also create a manual test runbook instance.

There's a lot here, so I'm open to discussing ways we can make it easier to review.

Also curious to see what people think we should do for a merge strategy, since there's a lot of commits and they come from two authors, but they really do want to all land on a unit (or at least, not along the subdivisions that the commits reflect).

This touches several tickets:

Outstanding "Auth0 v1" tickets:

issue description notes
#183 update auth endpoint to use DNS blocked; @AlaShiban needs to configure DNS
klothoplatform/klotho-auth-service#21 support http on auth server also blocked on @AlaShiban
klothoplatform/docs#167 document new user flow will do separately
klothoplatform/klotho-server#21 automated testing won't-fix (see ticket)

Standard checks

  • Unit tests: no special considerations
  • Docs: not documented, but needs to be: #244
  • Backwards compatibility: breaks existing login, but not existing deployments

Copy link
Author

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leaving a bit of a self-review :)

cmd/klotho/main.go Show resolved Hide resolved
pkg/auth/auth.go Show resolved Hide resolved
pkg/auth/auth.go Outdated Show resolved Hide resolved
Copy link
Contributor

@gordon-klotho gordon-klotho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing really blocking that we couldn't iterate on. Though we may want to while it's fresh

pkg/logging/fields.go Outdated Show resolved Hide resolved
pkg/logging/fields.go Outdated Show resolved Hide resolved
Comment on lines +8 to +16
func OrDebug(closer io.Closer) {
FuncOrDebug(closer.Close)
}

func FuncOrDebug(closer func() error) {
if err := closer(); err != nil {
zap.L().Debug("Failed to close resource", zap.Error(err))
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something I've used on side projects which might be nice (adapted to the style of this package):

func OrDebug(o any) {
	var err error
	switch o := o.(type) {
	case io.Closer:
		err = o.Close()
	case func() error:
		err = o()
	default:
		return
	}
	if err != nil {
		zap.L().Debug("Failed to close resource", zap.Error(err))
	}
}

pkg/cli/klothomain.go Outdated Show resolved Hide resolved
pkg/auth/auth.go Outdated Show resolved Hide resolved
pkg/auth/auth.go Outdated
State string
}

type Authorizer interface {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This interface should be defined in cli where it's used (ie, klothomain) not at definition time. The DefaultIfNil function can be moved there as well (or just inlined). Either that or move local authorizer in this package as a subpackage and "standardAuthorizer" (using the auth server) into another subpackage.

pkg/auth/auth.go Outdated
Comment on lines 55 to 42
func (s standardAuthorizer) Authorize() (*KlothoClaims, error) {
return Authorize()
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not how I'd have expected this to have been implemented, but I suppose it works.
What I'd expect (and IMO more idiomatic):

type ServiceAuthorizer struct {
	AuthUrlBase string
	PemUrl string
}

func (s ServiceAuthorizer) Authorize() (*KlothoClaims, error) {
	// auth implemented in this function
}

Login & Logout similarly implemented as methods and type-checked to call from klotho-main on the authorizer. This would enable other authorizer implementations to also have login/logout functionality.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to keep this as-is for now, since (a) it works, (b) refactoring it later is unlikely to be more work than refactoring it now, and (c) we currently don't have a need for multiple authorizers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(a) sure

(b) makes sense

(c) I'd argue that the local implementation is already multiple authorizers. If not, then there's no need for a struct or interface - just call the global methods from klothomain.

}

func CallLoginEndpoint() (string, error) {
res, err := http.Get(authUrlBase + "/login")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this should use an *http.Client and not the default one - though we don't do that a lot of other places so fine for now

@@ -30,184 +23,49 @@ type Validated struct {
// located in ~/.klotho/
var analyticsFile = "analytics.json"

func CreateUser(email string) error {

func GetOrCreateAnalyticsFile() AnalyticsFile {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like it should return an error

Copy link
Author

@ghost ghost Feb 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would you suggest we do with that error?

The code as it stands:

  1. Tries to get the current analytics.json file
  2. If that errors (probably because it doesn't exist, but possibly for other reasons) then:
    1. create a new tracking id (in the form of the AnalyticsFile struct); this can never error
    2. try to write that id to analytics.json; this can error, in which case we log a debug and proceed
    3. send that newly created tracking id up to the caller

If we returned an error from this method, I could imagine doing one of two things with it:

  1. logging it and proceeding, in which case that's the same as this currently logic
  2. erroring out to the user

Am I missing an option? If not, do you think we should do option 1 (in which case imo it's just a stylistic question) or 2 (in which case I disagree, but we should have that discussion)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regardless of (1) or (2) it doesn't feel like the analytics package should be the "decider" of that behaviour - so I guess it's a stylistic matter.

cmd/klotho/main.go Show resolved Hide resolved
@ghost
Copy link
Author

ghost commented Feb 17, 2023

Latest push is a rebase plus some fixup squashes. Here's the "diff of diffs" to show how this PR applies to main before and after the rebase:

$ diff <(git diff origin/auth main) <(git diff auth main)
42c42
< index 3aadacc..65dab23 100644
---
> index cd163da..65dab23 100644
59c59,61
< @@ -32,13 +31,6 @@ require (
---
> @@ -30,13 +29,6 @@ require (
>  	sigs.k8s.io/yaml v1.3.0
>  )
61,62d62
<  replace github.com/smacker/go-tree-sitter => github.com/klothoplatform/go-tree-sitter v0.1.1
<
69a70,71
>  replace github.com/smacker/go-tree-sitter => github.com/klothoplatform/go-tree-sitter v0.1.1
>
71,72d72
<  	github.com/Azure/go-ansiterm v0.0.0-20210617225240-d185dfc1b5a1 // indirect
<  	github.com/BurntSushi/toml v1.2.1 // indirect
90,98d89
< @@ -121,7 +112,7 @@ require (
<  	github.com/shopspring/decimal v1.2.0 // indirect
<  	github.com/sirupsen/logrus v1.9.0 // indirect
<  	github.com/spf13/cast v1.4.1 // indirect
< -	github.com/spf13/pflag v1.0.5
< +	github.com/spf13/pflag v1.0.5 // indirect
<  	github.com/stretchr/objx v0.5.0 // indirect
<  	github.com/xeipuuv/gojsonpointer v0.0.0-20180127040702-4e3ac2762d5f // indirect
<  	github.com/xeipuuv/gojsonreference v0.0.0-20180127040603-bd5ef7bd5415 // indirect

So the only changes were in go.mod and go.sum. The rebase just pulled in the latest from main (which makes sense).

@ghost ghost temporarily deployed to integ_test February 17, 2023 00:14 — with GitHub Actions Inactive
@ghost ghost had a problem deploying to integ_test February 17, 2023 00:14 — with GitHub Actions Failure
@ghost ghost temporarily deployed to integ_test February 17, 2023 00:33 — with GitHub Actions Inactive
@ghost ghost had a problem deploying to integ_test February 17, 2023 00:34 — with GitHub Actions Failure
Jordan Singer and others added 13 commits February 16, 2023 20:02
This introduces a new Authorizer, which is hookable per KlothoMain.
The default one just calls auth.Authorize(), but KlothoMains can provide
an alternative, including one that adds flags.

resolves #164
Before this, the server's /logintoken API would return instantly, with
either the token from auth0 or 404 if it hadn't been set yet from the
auth0.com callback. So, we needed a callback not just for intermittent
errors, but for the happy path functionality. Now, the /logintoken call
hangs until it gets that token (or times out), so the happy path doesn't
need a retry. We could still need a retry for intermittent failurs, but
that's a more general problem that we should resolve across all http
calls, not just for auth.

I'm also moving the Login error handler to a callback, just as an
organizational tool. We want to send the analytics for any error
manually (since the client hasn't been set up yet), but the Login method
shouldn't have to know about that intricacy. klothomain.go does, so
that's where we should put that logic
I'm doing it as a base (instead of just hostname) so that people can
specify http and port (especially for localhost testing).
In some cases, were creating the deferred close after the method would
have already returned. Also, we were dropping some errors on the ground;
this will now handle them.
- split analytics/client.go:NewClient up, such that its email stuff is
  in a new method, AttachAuthorizations().
- rm most of analytics/user.go, and replace it with a tiny method
  GetOrCreateAnalyticsFile(). This just upserts the
  ~/.klotho/analytics.json file. All of the logic around retrieving the
  user and validating emails and all that is just gone.
- auth/auth.go:
  - Authorize() now returns its claims. We pass these to
    AttachAuthorizations(), mentioned above.
  - We download our auth0 PEM (and cache it), and use it to verify the
    auth0 token. Without this, we can't actually get the claims.
- cli/klothomain.go:
  - construct analytics.Client just once, and then later attach the
    email from the claims (this resolves #180)
  - also move the log hooks up, to follow the client
- use RegisteredClaims instead of StandardClaims. Per the [jwt docs][1]:
   > Deprecated: Use RegisteredClaims instead for a forward-compatible way
   > to access registered claims in a struct.

[1]: https://pkg.go.dev/github.com/golang-jwt/jwt/v4#StandardClaims
If the `credentials.json` file isn't there at all, then require a login.
But if it's there but the login doesn't work for whatever reason, just
issue a warning and move on. Meanwhile, in the login path, handle errors
by writing an empty `credentials.json` and then ignoring the error.

resolves #194

Relatedly, add a workaround for #195. It's very easy to hit it with this
new path, so I felt like a quick workaround is in order.
@github-actions
Copy link

Package Line Rate Health
github.com/klothoplatform/klotho/pkg/analytics 3%
github.com/klothoplatform/klotho/pkg/annotation 23%
github.com/klothoplatform/klotho/pkg/cli 4%
github.com/klothoplatform/klotho/pkg/core 21%
github.com/klothoplatform/klotho/pkg/env_var 82%
github.com/klothoplatform/klotho/pkg/exec_unit 54%
github.com/klothoplatform/klotho/pkg/infra/kubernetes 59%
github.com/klothoplatform/klotho/pkg/infra/kubernetes/helm 39%
github.com/klothoplatform/klotho/pkg/input 63%
github.com/klothoplatform/klotho/pkg/lang 38%
github.com/klothoplatform/klotho/pkg/lang/dockerfile 0%
github.com/klothoplatform/klotho/pkg/lang/golang 33%
github.com/klothoplatform/klotho/pkg/lang/javascript 48%
github.com/klothoplatform/klotho/pkg/lang/python 61%
github.com/klothoplatform/klotho/pkg/lang/yaml 0%
github.com/klothoplatform/klotho/pkg/logging 6%
github.com/klothoplatform/klotho/pkg/multierr 95%
github.com/klothoplatform/klotho/pkg/provider/aws 60%
github.com/klothoplatform/klotho/pkg/runtime 78%
github.com/klothoplatform/klotho/pkg/static_unit 33%
github.com/klothoplatform/klotho/pkg/validation 73%
github.com/klothoplatform/klotho/pkg/yaml_util 79%
Summary 42% (3975 / 9568)

@ghost ghost merged commit c573b9c into main Feb 17, 2023
@ghost ghost deleted the auth branch February 17, 2023 01:08
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants